Fix mounting secrets #5063

yini7777 · 2024-03-15T13:19:20Z

Tracking issue

Closes: #5053

Why are the changes needed?

Context --> we're upgrading from v1.10.6 to v1.11.0 and nothing has been changed on our end. But got the following error:

│ Error: cannot patch "flytescheduler" with kind Deployment: Deployment.apps "flytescheduler" is invalid: spec.template.spec.initContainers[0].volumeMounts[2].name: Not found: "auth"
│ 
│   with helm_release.flyte-unstable-core,
│   on flyte-unstable.tf line 1, in resource "helm_release" "flyte-unstable-core":
│    1: resource "helm_release" "flyte-unstable-core" {

we set Values.secrets.adminOauthClientCredentials.enabled to false because we don't want Flyte to create the secret, as we have created it via an ExternalSecret . In this case Flyte does not mount the secret to the flytescheduler.

Solution --> Add a new value as Values.secrets.adminOauthClientCredentials.create . We will set this to false and enabled to true.

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

welcome · 2024-03-15T13:19:24Z

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
Sign off your commits (Reference: DCO Guide).

davidmirror-ops · 2024-03-15T15:37:29Z

@yini7777 I think a missing conditional for flytescheduler was added in #5054. Nevertheless I guess you're still facing the same issue as the conditional logic is not met when you set the adminOauthCredentials.enabled to false which not only doesn't create the secret but doesn't mount it.
Is my understanding correct?

codecov · 2024-03-15T18:35:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.11%. Comparing base (2528de7) to head (b256aaf).
Report is 7 commits behind head on master.

❗ Current head b256aaf differs from pull request most recent head be46ef6. Consider uploading reports for the commit be46ef6 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5063      +/-   ##
==========================================
+ Coverage   56.82%   59.11%   +2.28%     
==========================================
  Files          34      645     +611     
  Lines        2154    55574   +53420     
==========================================
+ Hits         1224    32852   +31628     
- Misses        837    20129   +19292     
- Partials       93     2593    +2500

Flag	Coverage Δ
unittests	`59.11% <ø> (+2.28%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yini7777 · 2024-03-18T09:40:48Z

Nevertheless I guess you're still facing the same issue as the conditional logic is not met when you set the adminOauthCredentials.enabled to false which not only doesn't create the secret but doesn't mount it. Is my understanding correct?

Yes exactly!

docs/deployment/configuration/auth_setup.rst

charts/flyte-core/values-keycloak-idp-flyteclients-without-browser.yaml

charts/flyte-core/templates/_helpers.tpl

yini7777 · 2024-03-22T07:36:53Z

@davidmirror-ops I'm wondering if there is anything needs to be tested or modified in this PR.

davidmirror-ops · 2024-03-22T15:19:03Z

@yini7777 Yesterday we merged #5072 to fix the lint-and-test-charts test. Also I see the generate Helm test failing

Could you please

Rebase to master and merge
Run make helm and push the changes

Let me know if you have any question

Thanks!

Signed-off-by: [email protected] <[email protected]> Signed-off-by: Yini <[email protected]>

Signed-off-by: Yini <[email protected]>

yini7777 · 2024-04-03T13:21:22Z

@wild-endeavor @davidmirror-ops Thank you for your update and explanation. I've made some modifications to align with your suggestions. Could you please review them?

davidmirror-ops

I deployed the changes proposed here and validated the behavior is as expected:

create & mount: specify clientSecret in the values file and enabled: true
don't create, but do mount: just set enabled: true
don't do anything: set enabled: false

yini7777 · 2024-04-05T07:45:23Z

@davidmirror-ops Thanks a lot for testing it out! Just curious about when validating don't create, but do mount: just set enabled: true, did u also overwrite the default clientSecret: foobar with clientSecret: null?

davidmirror-ops · 2024-04-08T16:31:08Z

@yini7777 I commented out that line entirely tbh

yini7777 · 2024-04-09T06:48:48Z

@davidmirror-ops Got it! I think it’s also crucial to test if overwriting with null works as expected, since from a user perspective it’s not practical to comment it out, I am also open to carry out this test myself.

The reason I’m so cautious about this is due to this issue. If you have any guidance on how to test it effectively, I’m eager to learn and contribute. Thanks!

davidmirror-ops · 2024-04-09T18:20:48Z

@yini7777 you nailed it. when I set

secrets:
  adminOauthClientCredentials:
    enabled: true
    clientSecret: null

propeller fails to start:

MountVolume.SetUp failed for volume "auth" : secret "flyte-secret-auth" not found

yini7777 · 2024-04-09T18:51:58Z

Awesome! Thanks a lot! Then I'll do a rebase and merge it!

Signed-off-by: Yini <[email protected]>

Signed-off-by: Yee Hing Tong <[email protected]>

Signed-off-by: Yini <[email protected]>

welcome · 2024-04-11T21:22:39Z

Congrats on merging your first pull request! 🎉

Signed-off-by: [email protected] <[email protected]> Signed-off-by: Andrew Dye <[email protected]>

yini7777 requested review from neverett and ppiegaze as code owners March 15, 2024 13:19

dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working documentation Improvements or additions to documentation labels Mar 15, 2024

yini7777 force-pushed the fix-mounting-secrets branch from 7e41838 to 1de6d0d Compare March 18, 2024 10:13

davidmirror-ops reviewed Mar 18, 2024

View reviewed changes

docs/deployment/configuration/auth_setup.rst Outdated Show resolved Hide resolved

davidmirror-ops reviewed Mar 18, 2024

View reviewed changes

charts/flyte-core/values-keycloak-idp-flyteclients-without-browser.yaml Outdated Show resolved Hide resolved

yini7777 commented Mar 19, 2024

View reviewed changes

charts/flyte-core/templates/_helpers.tpl Outdated Show resolved Hide resolved

yini7777 force-pushed the fix-mounting-secrets branch from 3e1d26e to 3e61e69 Compare March 19, 2024 17:07

davidmirror-ops requested a review from eapolinario March 19, 2024 17:23

[email protected] and others added 6 commits March 25, 2024 09:47

add 'create' key for adminOauthClientCredentials

fdc1097

Signed-off-by: [email protected] <[email protected]> Signed-off-by: Yini <[email protected]>

Update README.md

9c10c70

Signed-off-by: [email protected] <[email protected]> Signed-off-by: Yini <[email protected]>

update README

003c829

Signed-off-by: Yini <[email protected]>

rename 'enabled' to 'mount'

aca862f

Signed-off-by: Yini <[email protected]>

switch order of keys

759bf3d

Signed-off-by: Yini <[email protected]>

add comment to warn user the unsupported combo

6e6048d

Signed-off-by: Yini <[email protected]>

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Mar 25, 2024

yini7777 changed the base branch from master to move-to-monorepo/flyteplugins/dependabot/go_modules/boilerplate/flyte/golang_support_tools/github.com/prometheus/client_golang-1.11.1 March 25, 2024 10:18

yini7777 changed the base branch from move-to-monorepo/flyteplugins/dependabot/go_modules/boilerplate/flyte/golang_support_tools/github.com/prometheus/client_golang-1.11.1 to master March 25, 2024 10:18

run make helm

a17f82b

Signed-off-by: Yini <[email protected]>

yini7777 force-pushed the fix-mounting-secrets branch from 25a5830 to a17f82b Compare March 25, 2024 10:48

dosubot bot removed the size:L This PR changes 100-499 lines, ignoring generated files. label Mar 25, 2024

run make helm

b256aaf

Signed-off-by: Yini <[email protected]>

davidmirror-ops previously approved these changes Apr 4, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 4, 2024

Merge branch 'master' into fix-mounting-secrets

5f42484

yini7777 dismissed davidmirror-ops’s stale review via 5f42484 April 9, 2024 19:05

dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Apr 9, 2024

run make helm

e4a2dea

Signed-off-by: Yini <[email protected]>

dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Apr 9, 2024

davidmirror-ops requested a review from wild-endeavor April 9, 2024 19:16

wild-endeavor previously approved these changes Apr 11, 2024

View reviewed changes

Merge branch 'master' into fix-mounting-secrets

b867fac

Signed-off-by: Yee Hing Tong <[email protected]>

wild-endeavor dismissed their stale review via b867fac April 11, 2024 18:50

wild-endeavor previously approved these changes Apr 11, 2024

View reviewed changes

wild-endeavor enabled auto-merge (squash) April 11, 2024 18:56

run make helm

be46ef6

Signed-off-by: Yini <[email protected]>

auto-merge was automatically disabled April 11, 2024 20:00
Head branch was pushed to by a user without write access

yini7777 dismissed wild-endeavor’s stale review via be46ef6 April 11, 2024 20:00

wild-endeavor approved these changes Apr 11, 2024

View reviewed changes

wild-endeavor enabled auto-merge (squash) April 11, 2024 20:22

wild-endeavor merged commit 9955256 into flyteorg:master Apr 11, 2024
60 of 88 checks passed

troychiu pushed a commit that referenced this pull request Jul 8, 2024

Fix mounting secrets (#5063)

4f6d0e1

Signed-off-by: [email protected] <[email protected]> Signed-off-by: Andrew Dye <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mounting secrets #5063

Fix mounting secrets #5063

yini7777 commented Mar 15, 2024 •

edited

Loading

welcome bot commented Mar 15, 2024

davidmirror-ops commented Mar 15, 2024

codecov bot commented Mar 15, 2024 •

edited

Loading

yini7777 commented Mar 18, 2024

yini7777 commented Mar 22, 2024

davidmirror-ops commented Mar 22, 2024 •

edited

Loading

yini7777 commented Apr 3, 2024

davidmirror-ops left a comment

yini7777 commented Apr 5, 2024

davidmirror-ops commented Apr 8, 2024

yini7777 commented Apr 9, 2024

davidmirror-ops commented Apr 9, 2024

yini7777 commented Apr 9, 2024

welcome bot commented Apr 11, 2024

Fix mounting secrets #5063

Fix mounting secrets #5063

Conversation

yini7777 commented Mar 15, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

welcome bot commented Mar 15, 2024

davidmirror-ops commented Mar 15, 2024

codecov bot commented Mar 15, 2024 • edited Loading

Codecov Report

yini7777 commented Mar 18, 2024

yini7777 commented Mar 22, 2024

davidmirror-ops commented Mar 22, 2024 • edited Loading

yini7777 commented Apr 3, 2024

davidmirror-ops left a comment

Choose a reason for hiding this comment

yini7777 commented Apr 5, 2024

davidmirror-ops commented Apr 8, 2024

yini7777 commented Apr 9, 2024

davidmirror-ops commented Apr 9, 2024

yini7777 commented Apr 9, 2024

welcome bot commented Apr 11, 2024

yini7777 commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

davidmirror-ops commented Mar 22, 2024 •

edited

Loading